feat: toggle the enabled state of deployment targets - #694
NickJosevski wants to merge 8 commits into
Conversation
| return nil | ||
| } | ||
|
|
||
| selectedTarget, err := selectors.Select( |
There was a problem hiding this comment.
selectors.Select auto-returns the item when the list has exactly one entry (pkg/question/selectors/selectors.go:48), so in a space with a single deployment target, a bare interactive octopus deployment-target disable disables that target immediately with no prompt or confirmation at all. Note tenant disable goes through selectors.ByName → question.SelectMap, which always asks even for one item — so this command is both silently mutating and inconsistent with the existing enable/disable pattern.
There was a problem hiding this comment.
Actioned in 02ae422. The finding holds — selectors.Select returns items[0] without asking when len(items) == 1 (pkg/question/selectors/selectors.go:48-50), so a single-target space got no prompt.
PromptMissingTarget now calls question.SelectMap directly instead of going through selectors.Select, which lines it up with the tenant enable/disable behaviour: the list is always presented, one item or twenty. SelectMap is what selectors.Select falls through to anyway, so the only thing dropped is the auto-select shortcut. There's a comment at the call site saying why, so nobody reinstates selectors.Select for consistency later.
Covered by TestPromptMissingTarget_AsksEvenWhenThereIsOnlyOneTarget in pkg/cmd/target/shared/disabledstate_test.go, which feeds a one-target callback and asserts the select prompt is still asked (the mock asker's checkRemainingPrompts fails if it isn't).
| return err | ||
| } | ||
|
|
||
| opts.IdOrName = selectedTarget.GetID() |
There was a problem hiding this comment.
The prompt path fetches every machine in the space (take=2147483647) and the user picks a full *machines.DeploymentTarget, but only its ID is kept — SetDisabledState then re-fetches the same object via GetByIdentifier (an extra GET, or two when the identifier falls through to the name lookup). Carrying the selected target (e.g. a target *machines.DeploymentTarget field on the options, with GetByIdentifier only for the CLI-supplied identifier) drops a redundant API round-trip.
There was a problem hiding this comment.
Actioned in 7627672. SetDisabledStateOptions gained a Target *machines.DeploymentTarget field; PromptMissingTarget stores the object the user picked, and SetDisabledState only falls back to GetByIdentifier when Target is nil — i.e. when the identifier came from the command line. IdOrName is still set from the selection so the non-prompt path and any caller reading it keep working.
The prompt path is now list + PUT with nothing in between. That's asserted structurally rather than by a counter: in the prompts for the target when none was supplied cases in pkg/cmd/target/disable/disable_test.go and pkg/cmd/target/enable/enable_test.go, the mock server expects GET /machines?take=... then PUT /machines/Machines-200 directly — MockHttpServer fails on an unexpected request, so a re-added GET /machines/Machines-200 would break the test.
Residual: the list itself is still take=2147483647 over every machine in the space. That's GetTargetsOptions as target list and target delete already use it, so I left it alone; narrowing it would be a change to the shared helper rather than to this command.
|
|
||
| return &SetDisabledStateOptions{ | ||
| Dependencies: dependencies, | ||
| GetTargetsOptions: NewGetTargetsOptionsForAllTargets(dependencies), |
There was a problem hiding this comment.
The selection list is unfiltered, so disable offers already-disabled targets and enable offers already-enabled ones; picking one ends in the "is already disabled/enabled" no-op after the whole interactive flow. For enable, the machines endpoint supports server-side filtering (MachinesQuery{IsDisabled: true} — the query field is omitempty so only the true case can be expressed); for disable the list could be filtered client-side on !target.IsDisabled. Filtering would also make the single-item auto-select less surprising.
There was a problem hiding this comment.
Actioned in 62be612, using both halves of the suggestion.
NewSetDisabledStateOptions builds machines.MachinesQuery{IsDisabled: !disabled}, so enable sends isDisabled=true and disable sends nothing (the omitempty you noted). PromptMissingTarget then filters the returned list on target.IsDisabled != opts.Disabled regardless — the server-side filter only covers the enable case, and I didn't want the disable case relying on a filter the query type can't express. When nothing is left after filtering it errors with no deployment targets to <enable|disable> were found rather than dropping the user into an empty select.
Tests: does not offer targets which are already disabled in pkg/cmd/target/disable/disable_test.go returns one enabled and one disabled target and asserts the prompt options are ["web-server"] only; the enable equivalent asserts the request URL is /api/Spaces-1/machines?isDisabled=true&take=2147483647, which pins the server-side filter. TestPromptMissingTarget_ErrorsWhenNoTargetIsInTheOppositeState covers the empty case.
The "is already disabled/enabled" no-op message is kept — it's still reachable via an explicit identifier with --no-prompt.
| return err | ||
| } | ||
|
|
||
| fmt.Fprintf(opts.Out, "Successfully created listening tenatcle '%s'.\n", deploymentTarget.Name) |
There was a problem hiding this comment.
Pre-existing, but since this function is touched by the PR: user-visible typo "tenatcle" in the success message.
| fmt.Fprintf(opts.Out, "Successfully created listening tenatcle '%s'.\n", deploymentTarget.Name) | |
| fmt.Fprintf(opts.Out, "Successfully created listening tentacle '%s'.\n", deploymentTarget.Name) |
There was a problem hiding this comment.
Actioned in a3d40fb — took the suggestion as written. grep -rn tenatcle over the repo now returns nothing, so that was the only occurrence.
|
|
||
| const spaceID = "Spaces-1" | ||
|
|
||
| func newTarget(id string, name string, isDisabled bool) *machines.DeploymentTarget { |
There was a problem hiding this comment.
newTarget, rootResource, spaceID, and the whole table-test scaffold here are byte-for-byte duplicates of pkg/cmd/target/enable/enable_test.go. A shared helper (e.g. a deployment-target builder in test/fixtures next to fixtures.NewSpace) would keep the two files from drifting apart.
There was a problem hiding this comment.
Actioned in 314eafc. newTarget is gone from all three test files and replaced by fixtures.NewDeploymentTarget(spaceID, targetID, name, isDisabled) in test/fixtures/projects.go, next to NewSpace/NewTenant as suggested. There were actually three copies, not two — pkg/cmd/target/shared/disabledstate_test.go had a near-identical one as well; that's now on the shared fixture too.
Residual, deliberate: var rootResource = testutil.NewRootResource() and const spaceID = "Spaces-1" are still declared in each test package. They're package-level declarations rather than helpers, and the same pair appears in 29 test files across the repo, so hoisting them is a repo-wide change rather than something this PR should introduce a precedent for. The table-test scaffold itself (the tests []struct{name, run} slice and the for _, test := range tests runner) is likewise the standard shape used by every command test here — I kept it rather than abstracting it, since the two files' cases genuinely differ now that disable has the extra filtering case.
314eafc to
3c53db2
Compare
Adds `octopus deployment-target enable|disable [<name> | <id>]`, which flips `IsDisabled` on the machine and reports when the target is already in the requested state. The target is prompted for when no name or ID is supplied, matching the existing `tenant enable|disable` commands. Also adds a shared `--disabled` flag to every deployment-target create command so a target can be created in a disabled state, and includes it in the generated automation command. Dedicated subcommands rather than the `deployment-target update` command the issue suggests: `project` and `tenant` already ship `enable|disable`, and the CLI has no resource-level `update` command to follow. `--disabled` rather than `--enabled=false`: no bool flag in the CLI defaults to true, and flag names are states, not verbs. Fixes #311 Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Covers what MockHttpServer can't: the server honouring IsDisabled on create, and the read-modify-write PUT leaving the rest of the target's settings intact. Also pins that a worker ID is not accepted. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Second endpoint type, so the assertion is about the server honouring IsDisabled rather than about cloud regions specifically. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
selectors.Select returns the single item without asking when the list has exactly one entry, so in a space with one deployment target a bare `octopus deployment-target disable` mutated that target with no prompt at all. Ask through question.SelectMap instead, which always asks - matching `tenant enable|disable`, which reaches SelectMap via selectors.ByName. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
The prompt already loads every machine in the space and the user picks a whole *machines.DeploymentTarget, but only the ID was kept, so GetByIdentifier immediately fetched the same object again (and a second time when the ID lookup falls through to the name lookup). Carry the selected target on the options and only call GetByIdentifier for an identifier supplied on the command line. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
`disable` used to list already-disabled targets and `enable` already-enabled ones, so picking one ended in the "is already disabled/enabled" no-op after the whole interactive flow. Enable asks the machines endpoint for isDisabled=true (the query field is omitempty, so only the true case can be expressed) and both paths filter client-side, which also covers servers that ignore the query parameter. When nothing is eligible the command now says so instead of prompting. The desired state moves onto SetDisabledStateOptions so the target query can be built with it. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
Pre-existing, but in a line this PR already touches. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
`newTarget` was byte-for-byte identical in the enable and disable command tests, with a third near-copy in the shared package's test. Move it to test/fixtures next to NewSpace/NewTenant so the copies can't drift. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
3c53db2 to
db73ca7
Compare
Fixes #311
What changed
New commands —
octopus deployment-target enable [<name> | <id>]andoctopus deployment-target disable [<name> | <id>].Machines.GetByIdentifier, flipsIsDisabled, and saves viamachines.Update.--no-prompta missing identifier is an error.pkg/cmd/target/shared/disabledstate.go;enable/disableare thin cobra wrappers over it.New create flag —
--disabledon every deployment-targetcreatecommand (azure-web-app,cloud-region,kubernetes,listening-tentacle,ssh), registered from a sharedmachinescommon.CreateTargetDisabledFlagsand included in the generated automation command. It is flag-only: it deliberately does not add a prompt to the interactive create flow.Test support —
testutil.NewRootResource()now publishes theMachineslink so command tests can exercise the machines endpoints.Decisions
These were open questions in an earlier revision. They are settled, each on an existing CLI convention.
enable/disablesubcommands, not theupdatecommand the issue suggests.projectandtenantalready shipenable|disable, and the CLI has no resource-levelupdatecommand anywhere — the only two areproject variables updateandtenant variables update. A futuredeployment-target updatecan still cover the remaining editable fields; enable/disable stays the ergonomic shortcut.--disabled, not--enabled=falseor--disable. No bool flag in the CLI defaults totrue, so--enabled=falsewould be the first; and flag names are states (--prompted,--unscoped,--default), not verbs.--disabledalso matches theIsDisabledAPI field and theIS DISABLEDcolumntenant listalready prints.worker enable|disableplus--disabledon the worker create commands, with the shared helper generalised intopkg/machinescommon: feat: toggle the enabled state of workers #731, stacked on this branch.list/viewsurface the disabled state — followingtenant list/tenant view, for both targets and workers: feat: show the disabled state of deployment targets and workers #732, stacked on this branch.#731 and #732 are independent of each other and touch no files in common.
Test evidence
New tests:
pkg/cmd/target/enable/enable_test.go,pkg/cmd/target/disable/disable_test.go— end-to-end through the root command against the mock HTTP server, in the style ofrelease/progression/prevent: named target, already-in-state short circuit, and the interactive prompt path. The PUT body is asserted to carry the flippedIsDisabled.pkg/cmd/target/shared/disabledstate_test.go— prompting behaviour (identifier supplied vs not, and the enable/disable wording).pkg/cmd/target/target_test.go— the enable/disable subcommands are registered and every targetcreatecommand exposes--disabled.test/integration/target_test.go— against a real server:--disabledon two endpoint types, and that a toggle leaves the target's other settings untouched.Results from the worktree:
go build ./...— clean.go test ./pkg/...— all packages pass, no failures. (gofmt/go vetreport only pre-existing issues in files this PR does not touch.)No live server is required by the unit tests.
🤖 Generated with Claude Code